-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Move getDeclaredVariables and getAncestors to SourceCode #17059
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
lib/source-code/source-code.js
Outdated
* Gets all of the declared variables in the scope represented | ||
* by `node`. This is a convenience method that passes through |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Gets all of the declared variables in the scope represented | |
* by `node`. This is a convenience method that passes through | |
* Gets all of the variables declared by `node`. | |
* This is a convenience method that passes through |
I think "in the scope represented by node" would be confusing. For example, for function declarations, this returns variables for the function name and function parameters. The function name variable is not in the scope represented by node but in the parent scope. Also, variables declared at the top level of the function's body are directly in the scope represented by node, but this doesn't return them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While that is true, that seems like an edge case and otherwise the statement is accurate. I don't think "declared by node
" is accurate, as the node isn't doing any declaring. We use the node to look up the scope, and then look for variables in that scope. So, the scope associated with the node is where we look. Maybe I'll just replace "represented" with "associated"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use the node to look up the scope, and then look for variables in that scope. So, the scope associated with the node is where we look.
I still think the description would be confusing as it suggests that this returns all variables from the scope, but this returns only those that the given node creates.
For example:
{ // `blockStatement` node
let a, b; // `variableDeclaration1` node
let c, d; // `variableDeclaration2` node
}
This is a block scope associated with the blockStatement
node, variables a
b
c
d
are declared in that block scope, but getDeclaredVariables(blockStatement)
returns an empty array. getDeclaredVariables(variableDeclaration1)
returns [a, b]
(Variable
objects for a
and b
), getDeclaredVariables(variableDeclaration2)
returns [c, d]
.
description in eslint-scope
also uses wording "declared by the node".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this:
/**
* Gets variables declared directly by the provided node.
* This method looks up the scope associated with the node and returns only variables declared within that scope by the node itself.
* This may not include variables declared in parent or child scopes. If the node declares nothing, this method returns an empty array.
* @param {ASTNode} node - The node to use for scope lookup.
* @returns {Array<Variable>} - An array of variable nodes representing declared variables directly within the scope associated with the provided node.
*/
getDeclaredVariables(node) {
return this.scopeManager.getDeclaredVariables(node);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, it doesn't look up the scope but gets the variables from the cache by that particular node. When traversing a node that creates variables, eslint-scope
creates variable objects, caches them for that node, and inserts them into appropriate scope(s). For FunctionDeclaration
, named FunctionExpression
and ClassDeclaration
nodes, getDeclaredVariables()
returns variables from multiple scopes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mdjermanovic Keep in mind that eslint-scope was not written by a native English speaker, so I'm not sure that's really what we should be using as the source of truth in this case.
I'm not sure it's actually worth debating this given how few people will actually ever read these comments. Maybe let's just leave it as-is so we can merge the functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, we can discuss this in a follow-up. My concern is that someone reading these descriptions might think that sourceCode.getDeclaredVariables(node)
is the same as sourceCode.getScope(node).variables
, which isn't true.
lib/source-code/source-code.js
Outdated
* @returns {Array<Variable>} An array of variable nodes representing | ||
* the declared variables in the scope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @returns {Array<Variable>} An array of variable nodes representing | |
* the declared variables in the scope. | |
* @returns {Array<Variable>} An array of variables declared by `node`. |
Same as above.
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.37.0` -> `8.39.0`](https://renovatebot.com/diffs/npm/eslint/8.37.0/8.39.0) | --- ### Release Notes <details> <summary>eslint/eslint</summary> ### [`v8.39.0`](https://github.com/eslint/eslint/releases/tag/v8.39.0) [Compare Source](eslint/eslint@v8.38.0...v8.39.0) #### Features - [`3f7af9f`](eslint/eslint@3f7af9f) feat: Implement `SourceCode#markVariableAsUsed()` ([#​17086](eslint/eslint#17086)) (Nicholas C. Zakas) #### Documentation - [`6987dc5`](eslint/eslint@6987dc5) docs: Fix formatting in Custom Rules docs ([#​17097](eslint/eslint#17097)) (Milos Djermanovic) - [`4ee92e5`](eslint/eslint@4ee92e5) docs: Update README (GitHub Actions Bot) - [`d8e9887`](eslint/eslint@d8e9887) docs: Custom Rules cleanup/expansion ([#​16906](eslint/eslint#16906)) (Ben Perlmutter) - [`1fea279`](eslint/eslint@1fea279) docs: Clarify how to add to tsc agenda ([#​17084](eslint/eslint#17084)) (Nicholas C. Zakas) - [`970ef1c`](eslint/eslint@970ef1c) docs: Update triage board location (Nicholas C. Zakas) - [`6d8bffd`](eslint/eslint@6d8bffd) docs: Update README (GitHub Actions Bot) #### Chores - [`60a6f26`](eslint/eslint@60a6f26) chore: upgrade [@​eslint/js](https://github.com/eslint/js)[@​8](https://github.com/8).39.0 ([#​17102](eslint/eslint#17102)) (Milos Djermanovic) - [`d5ba5c0`](eslint/eslint@d5ba5c0) chore: package.json update for [@​eslint/js](https://github.com/eslint/js) release (ESLint Jenkins) - [`f57eff2`](eslint/eslint@f57eff2) ci: run tests on Node.js v20 ([#​17093](eslint/eslint#17093)) (Nitin Kumar) - [`9d1b8fc`](eslint/eslint@9d1b8fc) perf: Binary search in token store `utils.search` ([#​17066](eslint/eslint#17066)) (Francesco Trotta) - [`07a4435`](eslint/eslint@07a4435) chore: Add request for minimal repro to bug report ([#​17081](eslint/eslint#17081)) (Nicholas C. Zakas) - [`eac4943`](eslint/eslint@eac4943) refactor: remove unnecessary use of `SourceCode#getAncestors` in rules ([#​17075](eslint/eslint#17075)) (Milos Djermanovic) - [`0a7b60a`](eslint/eslint@0a7b60a) chore: update description of `SourceCode#getDeclaredVariables` ([#​17072](eslint/eslint#17072)) (Milos Djermanovic) - [`6e2df71`](eslint/eslint@6e2df71) chore: remove unnecessary references to the LICENSE file ([#​17071](eslint/eslint#17071)) (Milos Djermanovic) ### [`v8.38.0`](https://github.com/eslint/eslint/releases/tag/v8.38.0) [Compare Source](eslint/eslint@v8.37.0...v8.38.0) #### Features - [`a1d561d`](eslint/eslint@a1d561d) feat: Move getDeclaredVariables and getAncestors to SourceCode ([#​17059](eslint/eslint#17059)) (Nicholas C. Zakas) #### Bug Fixes - [`1c1ece2`](eslint/eslint@1c1ece2) fix: do not report on `RegExp(...args)` in `require-unicode-regexp` ([#​17037](eslint/eslint#17037)) (Francesco Trotta) #### Documentation - [`7162d34`](eslint/eslint@7162d34) docs: Mention new config system is complete ([#​17068](eslint/eslint#17068)) (Nicholas C. Zakas) - [`0fd6bb2`](eslint/eslint@0fd6bb2) docs: Update README (GitHub Actions Bot) - [`c83531c`](eslint/eslint@c83531c) docs: Update/remove external links, eg. point to `eslint-community` ([#​17061](eslint/eslint#17061)) (Pelle Wessman) - [`a3aa6f5`](eslint/eslint@a3aa6f5) docs: Clarify `no-div-regex` rule docs ([#​17051](eslint/eslint#17051)) (Francesco Trotta) - [`b0f11cf`](eslint/eslint@b0f11cf) docs: Update README (GitHub Actions Bot) - [`da8d52a`](eslint/eslint@da8d52a) docs: Update the second object instance for the "no-new" rule ([#​17020](eslint/eslint#17020)) (Ahmadou Waly NDIAYE) - [`518130a`](eslint/eslint@518130a) docs: switch language based on current path ([#​16687](eslint/eslint#16687)) (Percy Ma) - [`24206c4`](eslint/eslint@24206c4) docs: Update README (GitHub Actions Bot) #### Chores - [`59ed060`](eslint/eslint@59ed060) chore: upgrade [@​eslint/js](https://github.com/eslint/js)[@​8](https://github.com/8).38.0 ([#​17069](eslint/eslint#17069)) (Milos Djermanovic) - [`88c0898`](eslint/eslint@88c0898) chore: package.json update for [@​eslint/js](https://github.com/eslint/js) release (ESLint Jenkins) - [`cf682d2`](eslint/eslint@cf682d2) refactor: simplify new-parens rule schema ([#​17060](eslint/eslint#17060)) (MHO) - [`0dde022`](eslint/eslint@0dde022) ci: bump actions/add-to-project from 0.4.1 to 0.5.0 ([#​17055](eslint/eslint#17055)) (dependabot\[bot]) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4zNS4xIiwidXBkYXRlZEluVmVyIjoiMzUuNTUuMSJ9--> Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1852 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
This copies
getDeclaredVariables()
andgetAncestors()
to theSourceCode
class as a continuation of the work for the language plugins. I updated all existing rules to use the new methods.I also updated
getAncestors()
to cache its results. It's a little bit surprising that we weren't already doing caching here and just continued to calculate ancestry on every call. I also made the logic a little more efficient by removing the unnecessary call to.reverse()
.Refs #16999
Is there anything you'd like reviewers to focus on?
Did I miss any references?